Guard against nil attachments in processing form - #1539

Guards thesis processing against deleted Active Storage attachments.

Changes:

• Checks attachment existence before accessing its blob.
• Skips stale attachment entries safely.

The stale-attachment regression path lacks controller test coverage and should be tested.

Suppressed comments (1)

app/controllers/thesis_controller.rb:182

• This guard only skips building the flash-message entry; the same stale id remains in thesis_params and is passed to thesis.update at line 124. Because Thesis enables accepts_nested_attributes_for :files_attachments, Rails can still raise ActiveRecord::RecordNotFound while applying _destroy for an attachment that disappeared, so the reported 422 race is not fully prevented. Remove or ignore missing attachment entries before the update (and add a regression test for the stale id).

      next unless attachment

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Lite

Suppressed comments (2)

app/controllers/thesis_controller.rb:128

• This filter only recognizes the HTML string value '1'. accepts_nested_attributes_for treats other truthy forms such as 1/true as _destroy, so a stale attachment submitted with one of those values is not removed here and thesis.update can still raise ActiveRecord::RecordNotFound. Use the same boolean casting Rails uses for nested attributes (or normalize the value) before checking the missing attachment.

        marked_for_delete = attrs['_destroy'] == '1'
        attachment_id = attrs['id']
        missing_attachment = attachment_id.present? && !ActiveStorage::Attachment.exists?(attachment_id)

test/controllers/thesis_controller_test.rb:1015

• This regression test removes the row before the request, so the new exists? filter deletes the submitted attributes before deleted_file_list runs. As a result, the test still passes if the new find_by/next guard at lines 193-195 is removed and does not actually exercise the nil-attachment path it claims to cover. Please add an assertion or setup that specifically makes deleted_file_list receive a missing attachment (while separately covering the nested-update stale-row behavior).

    # Simulate the race condition: Delete the attachment from the database
    # (This could happen if another process deletes it between form open and submit)
    ActiveStorage::Attachment.find(attachment_id).delete

    # Attempt to update the thesis with the deleted attachment marked for deletion
    # This previously would crash with "undefined method 'blob' for nil:NilClass"
    patch "/thesis/#{thesis.id}/process",

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

Suppressed comments (2)

app/controllers/thesis_controller.rb:138

• The retry still leaves the same race window open: after the second drop_stale_deleted_attachment_rows! check, another process can delete the attachment before thesis.update, and because retried_for_missing_attachment is already true, that RecordNotFound is re-raised and the request still returns the 422 this change is intended to prevent. Consider handling the stale row atomically during the update or retrying/filtering until the submitted attachment rows are stable (with a bounded retry policy).

    rescue ActiveRecord::RecordNotFound => e
      raise unless missing_deleted_attachment_race?(e) && !retried_for_missing_attachment

      retried_for_missing_attachment = true
      drop_stale_deleted_attachment_rows!
      retry

test/controllers/thesis_controller_test.rb:1011

• This setup deletes the attachment before the request, so drop_stale_deleted_attachment_rows! removes the submitted row at lines 123–124 and deleted_file_list never receives a missing attachment. Consequently, the regression test would still pass if the new find_by guard were removed; please arrange the deletion between the stale-row check and deleted_file_list (or directly exercise that method) so the nil-attachment path is actually covered.

    # Simulate the race condition: Delete the attachment from the database
    # (This could happen if another process deletes it between form open and submit)
    ActiveStorage::Attachment.find(attachment_id).delete

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

Suppressed comments (3)

app/controllers/thesis_controller.rb:199

• The new next unless attachment branch is not exercised by either regression test: the first test removes the row before deleted_file_list runs, while the second removes it only during update after this method has already completed. Please add a deterministic test that deletes the attachment between the stale-row check and this lookup (or otherwise makes find_by return nil) so the actual nil-guard behavior is verified.

      attachment = ActiveStorage::Attachment.find_by(id: file['id'])
      next unless attachment

app/controllers/thesis_controller.rb:216

• This stale-row filter only recognizes the string '1', but nested-attribute requests can carry _destroy as an integer (1 is already used by the existing controller tests). For a missing attachment with that value, the row is not pruned, the first update raises, and the retry prunes nothing and raises again, so the request still fails. Use Rails' boolean casting (or normalize the value) here, consistently with nested-attribute semantics.

    params[:thesis][:files_attachments_attributes].delete_if do |_k, attrs|
      marked_for_delete = attrs['_destroy'] == '1'
      attachment_id = attrs['id']
      missing_attachment = attachment_id.present? && !ActiveStorage::Attachment.exists?(attachment_id)

test/controllers/thesis_controller_test.rb:1106

• This named route requires the thesis :id (see config/routes.rb:43), so calling thesis_process_path without an argument raises ActionController::UrlGenerationError while evaluating the assertion. Pass the thesis used by this test so the regression test can run.

    assert_redirected_to thesis_process_path

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

Suppressed comments (3)

test/controllers/thesis_controller_test.rb:1106

• This route requires the thesis :id parameter (config/routes.rb:43), so calling thesis_process_path without an argument raises ActionController::UrlGenerationError while evaluating the assertion. Pass the thesis, as the preceding regression test does, so this test can reach its response assertions.

    assert_redirected_to thesis_process_path

test/controllers/thesis_controller_test.rb:1007

• has_many_attached :files returns blobs, so thesis.files.first.id is the blob ID rather than the ActiveStorage::Attachment ID expected by files_attachments_attributes. The test can therefore delete a different attachment (or fail to find one), meaning it does not reliably exercise the stale-attachment path. Use thesis.files_attachments.first.id (or capture the attachment returned by the association) here.

    # Get the attachment ID to mark for deletion
    attachment_id = thesis.files.first.id

test/controllers/thesis_controller_test.rb:1059

• The race test has the same blob/attachment ID mismatch: thesis.files.first.id is a blob ID, while the nested attributes and ActiveStorage::Attachment.find_by call require the thesis attachment ID. As a result, the wrapper may delete a transfer attachment or no record, so the asserted retry path is not deterministic. Capture thesis.files_attachments.first.id instead.

    attach_files_to_records(transfer, thesis)

    attachment_id = thesis.files.first.id

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

Suppressed comments (1)

app/controllers/thesis_controller.rb:200

• The new nil guard is not exercised by either regression test: the first test deletes the row before drop_stale_deleted_attachment_rows! runs, while the second deletes it inside update, after deleted_file_list has already completed. Add a deterministic test that lets the initial stale-row check observe the attachment and then removes it immediately before deleted_file_list calls find_by, so this specific failure mode is covered.

      attachment = ActiveStorage::Attachment.find_by(id: file['id'])
      next unless attachment

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite